Scheduler - Replace action enum with onDone/onCancel callbacks in AppointmentPopup#33047
Scheduler - Replace action enum with onDone/onCancel callbacks in AppointmentPopup#33047aleksei-semikozov wants to merge 4 commits intoDevExpress:26_1from
Conversation
There was a problem hiding this comment.
Pull request overview
Updates Scheduler’s AppointmentPopup integration to remove action-enum–driven behavior and instead delegate save behavior to a provided callback (plus new config-driven title/readOnly behavior).
Changes:
- Replaced
ACTION_TO_APPOINTMENT-based save branching inAppointmentPopupwith a singleconfig.onDone(...)callback. - Updated
Schedulerpopup invocations to passonDone,title, andreadOnly(while keeping legacy-form behavior on the legacy popup). - Refactored/expanded Jest tests and the popup test harness mock to validate callback invocation, title rendering, and read-only behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
packages/devextreme/js/__internal/scheduler/m_scheduler.ts |
Switches non-legacy popup usage to callback-based onDone flows and sets popup title/readOnly from scheduler context. |
packages/devextreme/js/__internal/scheduler/appointment_popup/m_popup.ts |
Introduces AppointmentPopupConfig and replaces action-based save logic with config.onDone; uses config.title and config.readOnly. |
packages/devextreme/js/__internal/scheduler/appointment_popup/appointment_popup.test.ts |
Updates tests to validate onDone, title rendering, and Save visibility under readOnly. |
packages/devextreme/js/__internal/scheduler/__tests__/__mock__/create_appointment_popup.ts |
Updates the test factory to pass the new config shape (onDone/title/readOnly) into popup.show. |
| updateAppointment?: jest.Mock; | ||
| onDone?: jest.Mock; | ||
| title?: string; | ||
| readOnly?: boolean; |
There was a problem hiding this comment.
CreateAppointmentPopupOptions no longer declares addAppointment / updateAppointment, but createAppointmentPopup still reads options.addAppointment and options.updateAppointment later in this file. This will cause a TypeScript error (properties do not exist on the options type). Either re-add these optional fields to the interface, or remove the reads and rely solely on onDone for test customization.
| readOnly?: boolean; | |
| readOnly?: boolean; | |
| addAppointment?: jest.Mock; | |
| updateAppointment?: jest.Mock; |
| onDone: (newAppointment) => { | ||
| this.updateAppointment(rawAppointment, appointment.source); | ||
| return this.addAppointment(newAppointment); | ||
| }, | ||
| title: messageLocalization.format('dxScheduler-editPopupTitle'), | ||
| readOnly: false, | ||
| isToolbarVisible: true, | ||
| }; |
There was a problem hiding this comment.
In the non-legacy exclude-from-series popup config, readOnly is hard-coded to false. Previously, the popup became read-only for disabled appointments, and checkRecurringAppointment only guards allowUpdating (not disabled). Consider computing readOnly here the same way as the regular update flow (e.g., based on AppointmentAdapter(...).disabled and editing permissions) to avoid enabling edits for disabled occurrences.
| export interface AppointmentPopupConfig { | ||
| onDone: (appointment: Record<string, unknown>) => PromiseLike<unknown>; | ||
| title: string; | ||
| readOnly: boolean; | ||
| isToolbarVisible?: boolean; | ||
| } |
There was a problem hiding this comment.
The PR title mentions replacing the action enum with onDone/onCancel callbacks, but the new AppointmentPopupConfig only adds onDone (no onCancel). Either add an onCancel callback to the config and wire it to the Cancel action, or adjust the PR title/scope so it matches the implemented API.
| onDone: (appointment: Record<string, unknown>) => PromiseLike<unknown>; | ||
| title: string; | ||
| readOnly: boolean; | ||
| isToolbarVisible?: boolean; |
There was a problem hiding this comment.
AppointmentPopupConfig defines isToolbarVisible?, and call sites in m_scheduler.ts set it, but AppointmentPopup never reads this value (the only occurrence is the interface field). This is dead API surface and can confuse callers; either implement the flag (e.g., to control Done/Cancel toolbar items) or remove it from the config and stop passing it.
| isToolbarVisible?: boolean; |
aleksei-semikozov
force-pushed
the
scheduler-popup-callbacks
branch
from
5c8b946 to
9c52ce8
Compare
…adOnly in exclude-from-series, fix mock types
No description provided.